Skip to content

review E: follow-up review docs/30 (R1-R9) - #66

Merged
RobbinBouwmeester merged 2 commits into
review/d-provenancefrom
review/e-followup
Sep 8, 2026
Merged

review E: follow-up review docs/30 (R1-R9)#66
RobbinBouwmeester merged 2 commits into
review/d-provenancefrom
review/e-followup

Conversation

@RobbinBouwmeester

Copy link
Copy Markdown
Member

Fifth stacked review PR (docs/30_code_review_2026-09-08.md, all nine findings). Base is review/d-provenance (#65); merge #61, A, B, C, D, then this.

Findings

Change Test
R1 (P1, regression from A) finetune_batch = 0 is the automatic batch size again; only finetune_epochs >= 1 is required when the fine-tune is on enabling_the_fine_tune_keeps_the_automatic_batch: omitted, zero and positive batch accepted, zero epochs rejected
R2 (P1) portable_dir_name_problem: trailing dots and spaces, the characters `<>:" ?*` and control characters, Windows reserved device names (with or without extension) rejected on every platform, before anything is written
R3 (P1) cancel is intent and kill only, inert once terminal, holds the pid lock across the kill; publish_exit retires the pid under that lock, sweeps, then releases; the progress thread writes only while active (set_if_active) a_late_stop_after_the_run_ended_leaves_the_folder_alone (another owner's temp file survives), a_stop_still_in_flight_finishes_before_the_folder_is_released (reservation held while the kill is in flight)
R4 (P1) unique partial names <stem>.partial-<pid>-<n>.mzML and a <destination>.converting lock; a concurrent converter waits and reuses the finished mzML; a lock whose holder stopped writing for 15 min is broken a_conversion_lock_is_exclusive_and_released_on_drop, a_stale_lock_is_broken_and_a_fresh_one_is_honoured
R5 domain checks for the remaining numeric fields (fractions, q-values, correlations, percentile, tolerances, widths, counts, folds >= 2, promote_top_peaks <= retain_top_peaks, power-of-two bucket size), boundaries tested as accepted numeric_domains_are_validated_at_load
R6 a prediction count that differs from the batch is a hard error; rows without a finite prediction keep the imported iRT and are counted in <lib_out>.summary.json; the engine warns when the count is non-zero worker rewrite_irt; warn_on_retained_imported
R7 reported = rejection_reason == REPORTED; a decoy past both gates is REMOVED_DURING_REPORTING; a present precursor_q of the wrong type is an error, only an absent column falls back the_reported_flag_follows_the_reason_and_the_report_rules, a_present_but_malformed_precursor_q_column_is_an_error_not_a_fallback
R8 reservations refuse an ancestor or a descendant of an active folder, by path components overlapping_result_folders_are_refused_in_both_orders_but_siblings_are_not
R9 the CLI runs on a thread with a 256 MiB stack reservation tests/cli_version.rs runs the built binary's --version and --help

Cleanup notes: model identities carry the installed versions (deeplc-4.1.1-base, deeplc-4.1.1-finetuned, ms2pip-4.2.0-HCDch2); the MBR worker writes a full-schema empty transfer table and an unflagged augmented scored table when there is nothing to transfer (test updated); the desktop progress thread no longer writes after publication.

Pushback, recorded in the docs/30 status section

  • R6: rows are not dropped from the rewritten library. candidate_id is the contiguous index the fragment table references, so dropping would mean renumbering both tables for a rare event. A structural mismatch errors; retained imported values are counted and warned about.
  • R4: no recipe-keyed conversion cache; unique temporary names plus the destination lock cover the reproduced failure. Two different recipes aimed at one destination remain a documented limitation of writing beside the input.
  • Checkpoint hashes in model identities are deferred; versions are recorded.

Local checks

Engine cargo fmt --check, clippy -D warnings, cargo test --workspace (266 tests, including the new binary subprocess tests); desktop clippy + cargo test --lib (81); pytest tests/python/test_mbr_worker.py tests/python/test_predictor_workers.py (29 passed, 3 skipped); python -m compileall scripts; ci/gen_config_reference.py regenerated (line shifts only); ci/check_doc_refs.py; ci/check_desktop_ui.py. The debug binary's --version overflow was reproduced on this machine before the change.

🤖 Generated with Claude Code

RobbinBouwmeester and others added 2 commits September 8, 2026 09:30
…rtable run names, cancel lifetime, conversion ownership, numeric domains, rewrite coverage, audit acceptance, nested reservations, main-thread stack

Code review E (docs/30_code_review_2026-09-08.md, findings R1 to R9) plus the
cleanup notes on model identity and the MBR worker's empty outputs.

R1. Enabling DeepLC fine-tuning with its own defaults was rejected at load,
because finetune_batch = 0 is the documented automatic batch size and review A's
validation demanded a positive batch. Only the epoch count is bounded now.

R2. run-experiment --run-names accepted `a` and `a.`, one directory on Windows,
and the second run overwrote the first with exit 0. Names ending in a dot or a
space, containing <>:"|?* or a control character, or naming a Windows reserved
device are rejected on every platform before anything is written.

R3. A desktop stop could sweep temporary files that belonged to the next run in
the same folder: the sweep ran after the reservation was released, and a stop on
a finished run swept too. Cancellation is intent and kill only, inert once the
run is terminal, and holds the pid lock across the kill; the waiter retires the
pid under that lock, sweeps, then releases, so a stop still killing finishes
before the folder changes hands. The progress thread writes only while active.

R4. Two searches converting the same vendor file concurrently shared one
temporary output and one could publish the other's bytes. Each conversion writes
a unique partial file under a lock beside the destination; a concurrent
converter waits and reuses the result; a lock whose holder stopped writing for
fifteen minutes is broken.

R5. Domain checks for the numeric settings review A left unchecked, with the
boundaries tested as accepted.

R6. The fine-tune and re-prediction worker zipped predictions with peptidoforms
without checking the count and silently kept the imported iRT for anything it
did not predict. A count mismatch is an error; rows keeping their imported value
are counted in <lib_out>.summary.json and the engine warns when there are any.
Rows are not dropped: candidate_id is the contiguous index the fragment table
references, and renumbering both tables for a rare event is a worse risk.

R7. The audit's reported flag repeated the precursor gate; it follows the
rejection reason now, a decoy past both gates is REMOVED_DURING_REPORTING, and a
present precursor_q column of the wrong type is an error rather than a fallback.

R8. The desktop reservation compared exact folders only; ancestors and
descendants of an active folder are refused, siblings are not.

R9. The Windows debug binary overflowed its 1 MiB main-thread stack on
--version. The CLI runs on a thread with a 256 MiB reservation, and
tests/cli_version.rs runs the built binary.

Model identities carry the installed DeepLC and MS2PIP versions. With nothing to
transfer the MBR worker writes a full-schema empty transfer table and the
requested augmented scored table unflagged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@RobbinBouwmeester
RobbinBouwmeester merged commit 12cc9f9 into review/d-provenance Sep 8, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant